http: close pre-request sockets in closeIdleConnections#63470
Conversation
|
Review requested:
|
Signed-off-by: semimikoh <ejffjeosms@gmail.com>
2fa5144 to
95c435e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #63470 +/- ##
==========================================
- Coverage 91.67% 90.05% -1.62%
==========================================
Files 361 714 +353
Lines 156408 225925 +69517
Branches 24050 42732 +18682
==========================================
+ Hits 143384 203463 +60079
- Misses 12751 14238 +1487
- Partials 273 8224 +7951
🚀 New features to boost your workflow:
|
pimterry
left a comment
There was a problem hiding this comment.
Thanks for opening this @semimikoh! I'd mildly prefer to do this within ConnectionList itself (see comment) as that'd be a bit cleaner, but I could be persuaded if there's a big issue with that.
| @@ -681,6 +681,7 @@ Server.prototype.closeIdleConnections = function closeIdleConnections() { | |||
| } | |||
|
|
|||
| const connections = this[kConnections].idle(); | |||
There was a problem hiding this comment.
This approach does look like it works fine, but I wonder if it's changing the wrong level. Could we just make idle() return these connections as well? That would be clearer, and avoid hitting the same issue if we use idle() anywhere else.
I think this is fairly doable: I'd suggest adding a boolean field like received_data_ in the parser, set to false in Initalize, and then true in on_message_begin. Single boolean & write, so very cheap, and then ConnectionsList::Idle can return any connections where it's still false.
What do you think?
Problem
server.closeIdleConnections()does not close TCP connections that have been accepted but have not yet sent any HTTP data, contradicting itsdocumented behavior ("Closes all connections connected to this server which are not
sending a request or waiting for a response") and blocking graceful shutdown when peers (e.g. browsers opening speculative connections) hold
sockets open without writing.
Repro:
Before:
still hung after 2s. After: closes immediately.Reported in #63452.
Cause
New sockets are pushed into
kConnectionsfromconnectionListenerInternalviaparser.initialize(...), butParser::Initialize(
src/node_http_parser.cc:730) setslast_message_start_ = uv_hrtime()so thatheadersTimeout/requestTimeoutcan begin counting (DoSprotection).
ConnectionsList::Idle()(src/node_http_parser.cc:1147) only returns parsers withlast_message_start_ === 0, which becomes trueonly after
on_message_completeresets it (line 544). Sockets that were accepted but have not sent any HTTP data are therefore classified as activeand skipped by
closeIdleConnections().Fix
lib/_http_server.js: in addition to the existingidle()iteration, walkall()and destroy any socket whosebytesRead === 0. That uniquelyidentifies sockets that have not received any data and so cannot be sending a request or awaiting a response. Keep-alive sockets between requests,
in-flight requests, and in-flight responses all have
bytesRead > 0and are unaffected.Test
Added
test/parallel/test-http-server-close-idle-connections-pre-request.js. It opens a TCP connection without sending any data, callsserver.close()+server.closeIdleConnections(), and uses an unref'dsetTimeout(common.mustNotCall(), 1000)as the safety net: if the fix worksthe event loop drains and the timer never fires; without the fix the timer fires and the assertion trips.
Verification
A full local build of the patched binary was not completed because this machine has Apple clang 16.0.0, while the current tree requires a newer
toolchain (V8 fails to compile due to missing
std::atomic_ref). The behavior change was verified by monkey-patchingServer.prototype.closeIdleConnectionswith the same logic and running the issue's reproduction.Fixes: #63452